Skip to content

Biopython update - #157

Open
pipaj97 wants to merge 13 commits into
masterfrom
biopython_update
Open

Biopython update#157
pipaj97 wants to merge 13 commits into
masterfrom
biopython_update

Conversation

@pipaj97

@pipaj97 pipaj97 commented Jun 25, 2023

Copy link
Copy Markdown
Collaborator

Description

In the latest version of biopython, some functionalities do not work anymore in OpenCADD.
This PR fixes this problem.

Todos

Notable points that this PR has either accomplished or will accomplish.

  • Update sequence functionalities to work with the latest biopython version
  • add a test for the newly implemented gap function
  • Check and solve CI failures

Status

  • Ready to go

@pipaj97 pipaj97 added the enhancement New feature or request label Jun 25, 2023
@pipaj97
pipaj97 requested a review from AAriam June 25, 2023 12:10
@pipaj97 pipaj97 added the module-structure-superposition Concerns opencadd.structure.superposition module label Jun 25, 2023
@AAriam
AAriam changed the base branch from dev to master January 30, 2024 09:17
@AAriam AAriam closed this Jan 30, 2024
@AAriam AAriam reopened this Jan 30, 2024

@AAriam AAriam left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were some apparent mistakes that I commented on the opencadd/structure/superposition/sequences.py file.

However, more importantly, the fasta2select function (i.e. the only part of the code being changed in this PR) does not have any explicit unit tests. Therefore, even after resolving these comments, we can't be sure of the correctness of the function until we write some explicit test-cases.

Comment thread opencadd/structure/superposition/sequences.py
s = np.zeros((nseq, alignment.get_alignment_length()), dtype=object)
for iseq, a in enumerate(alignment):
GAP = a.seq.alphabet.gap_char
print(a.seq)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover from debugging?

ref_selection = " or ".join(sel[0])
target_selection = " or ".join(sel[1])
return {"reference": ref_selection, "mobile": target_selection}
return {"reference": ref_selection, "mobile": target_selection} No newline at end of file

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add end-of-file newline

Comment thread opencadd/structure/superposition/sequences.py
@AndreaVolkamer

Copy link
Copy Markdown
Member

@pipaj97 can you have a look at these comments please?

@AndreaVolkamer

Copy link
Copy Markdown
Member

@dominiquesydow can you please have a look in the CI failures realted to the kliffs package:

 =========================== short test summary info ============================
  FAILED opencadd/tests/databases/test_klifs_local_remote.py::TestsAllQueries::test_all_interactions - bravado.exception.HTTPURITooLong: 414 Request-URI Too Long: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
  <html><head>
  <title>414 Request-URI Too Long</title>
  </head><body>
  <h1>Request-URI Too Long</h1>
  <p>The requested URL's length exceeds the capacity
  limit for this server.<br />
  </p>
  <hr>
  <address>Apache/2.4.38 (Debian) Server at klifs.net Port 443</address>
  </body></html>
  FAILED opencadd/tests/databases/test_klifs_local_remote.py::TestsAllQueries::test_all_conformations - bravado.exception.HTTPURITooLong: 414 Request-URI Too Long: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
  <html><head>
  <title>414 Request-URI Too Long</title>
  </head><body>
  <h1>Request-URI Too Long</h1>
  <p>The requested URL's length exceeds the capacity
  limit for this server.<br />
  </p>
  <hr>
  <address>Apache/2.4.38 (Debian) Server at klifs.net Port 443</address>
  </body></html>

@dominiquesydow

dominiquesydow commented Feb 25, 2024

Copy link
Copy Markdown
Contributor

Hi @AndreaVolkamer,

I fixed the following two items:

1. A future deprecation warning for pandas, e.g. here:

/Users/dominique/Documents/GitHub/opencadd/opencadd/databases/klifs/core.py:147: FutureWarning: A value is trying to be set on a copy of a DataFrame or Series through chained assignment using an inplace method.
  The behavior will change in pandas 3.0. This inplace method will never work because the intermediate object on which we are setting values always behaves as a copy.
  
  For example, when doing 'df[col].method(value, inplace=True)', try using 'df.method({col: value}, inplace=True)' or df[col] = df[col].method(value) instead, to perform the operation inplace on the original object.
  
  
    dataframe["ligand_allosteric.expo_id"].replace(0, "-", inplace=True)

We are doing now instead:

dataframe["ligand_allosteric.expo_id"] = dataframe["ligand_allosteric.expo_id"].replace(0, "-")

2. Two instances of HTTPURITooLong errors when querying KLIFS.

In these two cases we are pulling all interactions and all conformations - part of that query is that we pull all structure IDs from KLIFS, to then build a URL with all of them to retrieve interactions and conformations. This URL is naturally too long (I guess they rightfully added a limit). I am now chunking over the list of structure IDs instead.

@codecov-commenter

codecov-commenter commented Feb 25, 2024

Copy link
Copy Markdown

Codecov Report

Merging #157 (cb3a39f) into master (c76e87c) will increase coverage by 0.10%.
The diff coverage is 91.42%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@dominiquesydow

Copy link
Copy Markdown
Contributor

@AndreaVolkamer I did not touch any KLIFS-unrelated issues.

@dominiquesydow

Copy link
Copy Markdown
Contributor

@AndreaVolkamer ok could not resist & applied black to the full package --- can someone look into Python 3.12, which seems to fail with conda.

@AndreaVolkamer

Copy link
Copy Markdown
Member

@AAriam since this was stale for a while (1.5 years) can you please check if we need further fixes, so we can merge the PR.

@AAriam

AAriam commented Feb 26, 2025

Copy link
Copy Markdown
Collaborator

So this PR now contains two unrelated changes: one is fixes to the KLIFS subpackage by Dominique, and the other is updating the Biopython version. For the latter, I had already left a review pointing to some problems, which are not yet resolved. Also, as we discussed in our last meeting, the Biopython problem is only a part of the problem with the superposition subpackage.
I think the best approach is to separate these two fixes, and first apply the KLIFS fix, which is already working. Also, depending on your decision on how to proceed with separating the two subpackages, there are other updates that need to be done, but first we need to decide on our strategy.
As I mentioned earlier, the best approach in my opinion is to release a new version of the KLIFS subpackage as a standalone package on PyPI. This way the updated KLIFS part will be immediately available to downstream users. I can do this in a couple of hours if you want.

Comment thread .github/workflows/ci.yaml Outdated
Comment thread opencadd/structure/superposition/sequences.py Outdated
@AndreaVolkamer

Copy link
Copy Markdown
Member

@AAriam in the interest of getting the whole infrastructure running again, let's keep the package as is, implement the fixes, and do the separation, if needed, later. Could you have a look at the CI failure, please?

@AAriam

AAriam commented Mar 14, 2025

Copy link
Copy Markdown
Collaborator

@AndreaVolkamer I added the updates to the CI pipeline in #164. Please squash-merge into main, then we can pull here.

gph82 and others added 2 commits October 2, 2025 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request module-structure-superposition Concerns opencadd.structure.superposition module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants